-
Notifications
You must be signed in to change notification settings - Fork 142
Improve connection list step prompts #4627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/commands/appSettings/connectionSettings/azureWebJobsStorage/StorageConnectionListStep.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few UX/consistency comments on the updated modal prompts (storage, DTS, Event Hubs, SQL). No blocking issues; suggestions focus on phrasing, capitalization, and adding brief rationale/docs to reduce confusion in a modal flow.
Review generated with Copilot
@@ -46,7 +46,7 @@ export class StorageConnectionListStep<T extends IStorageConnectionWizardContext | |||
|
|||
buttons.push(skipForNow); | |||
|
|||
const message: string = localize('connectAzureWebJobsStorage', 'In order to proceed, you must connect a storage account for internal use by the Azure Functions runtime.'); | |||
const message: string = localize('connectAzureWebJobsStorage', 'Azure Functions requires a storage account to be configured.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX: Shortening this message removes the rationale ("for internal use by the Azure Functions runtime"). Consider retaining a brief rationale or adding a "Learn more" link, since this is a modal blocking step. Also ensure this prompt does not appear when azureWebJobsStorage
is already configured to avoid confusion.
Review generated with Copilot
@@ -26,7 +26,7 @@ export class DTSConnectionListStep<T extends IDTSConnectionWizardContext> extend | |||
} | |||
|
|||
public async prompt(context: T): Promise<void> { | |||
const connectAzureButton = { title: localize('connectAzureTaskScheduler', 'Connect Azure Task Scheduler'), data: ConnectionType.Azure }; | |||
const connectAzureButton = { title: localize('connectDurableTaskScheduler', 'Connect Durable Task Scheduler'), data: ConnectionType.Azure }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming: The button title uses "Durable Task Scheduler" while other parts sometimes say "Task Scheduler". Please standardize across UI strings and localize keys (connectDurableTaskScheduler
vs connectAzureTaskScheduler
).
Review generated with Copilot
@@ -44,7 +44,7 @@ export class DTSConnectionListStep<T extends IDTSConnectionWizardContext> extend | |||
|
|||
buttons.push(skipForNow); | |||
|
|||
const message: string = localize('selectDTSConnection', 'Durable Functions needs to be configured to use a Durable Task Scheduler.'); | |||
const message: string = localize('selectDTSConnection', 'Durable Functions requires a Durable Task Scheduler to be configured.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording: "requires ... to be configured" is consistent with other prompts, but consider adding a concise rationale or a doc link. Users often don't know why this is needed for Durable Functions (hub state storage).
Review generated with Copilot
@@ -38,7 +38,7 @@ export class EventHubsConnectionListStep<T extends INetheriteConnectionWizardCon | |||
|
|||
buttons.push(skipForNow); | |||
|
|||
const message: string = localize('selectEventHubsNamespace', 'In order to proceed, you must connect an event hubs namespace for internal use by the Azure Functions runtime.'); | |||
const message: string = localize('selectEventHubsNamespace', 'Durable Functions requires an Event Hubs Namespace to be configured.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency: "Event Hubs Namespace" is capitalized here. Ensure consistent capitalization across prompts (e.g., "SQL database" is lowercased below). Either treat product names as proper nouns or use sentence case everywhere.
Review generated with Copilot
@@ -35,7 +35,7 @@ export class SqlConnectionListStep<T extends ISqlDatabaseConnectionWizardContext | |||
|
|||
buttons.push(skipForNow); | |||
|
|||
const message: string = localize('selectSqlDatabaseConnection', 'In order to proceed, you must connect a SQL database for internal use by the Azure Functions runtime.'); | |||
const message: string = localize('selectSqlDatabaseConnection', 'Durable Functions requires a SQL database to be configured.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording/capitalization: Here we say "SQL database" (lowercase database) while Event Hubs uses title case. Please align casing and consider clarifying whether Azure SQL specifically is required or any SQL-compatible storage for the Durable Task SQL provider.
Review generated with Copilot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the generated comments are worth considering but mostly are nits.
No description provided.